Various fixes - #1
Conversation
A zero was indistinguishable from an omitted key, so every field whose zero value is a legitimate setting was silently replaced by its default: zipf: 0 -> 1.10 (README documents 0 as "equal base weights") total_noise: 0 -> 0.015 (noise could not be disabled) host_noise: 0 -> 0.25 correlation: 0 -> 0.98 (uncorrelated noise was unreachable) stddev: 0 -> 220 (constant frame size was unreachable) seed: 0 -> 1 sub_agent_id: 0 -> 1 in attack mode Make those fields pointers in the YAML structs and resolve the defaults only when the key is absent. RuntimePacket carries the resolved packet settings so the rest of the code keeps working with plain values. Fields whose zero value is invalid anyway (sizes, sampling_rate, tick, samples_per_datagram, hosts, max_samples_per_tick) are left as they are. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
uint16Bytes had no callers, and both branches of the AddrFromSlice conditional in addressAt were identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The MAC discriminator was chosen by the role of the address in the packet (0x11 source, 0x22 destination) rather than by its role in the topology, so each host appeared with two different MAC addresses depending on direction, and every MAC appeared to move between switch ports. Derive it from the internal/peer role instead, which is stable across directions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
attackPhase returned "ramp-down" for everything past the hold phase, including configurations without a ramp_down and the final tick after the attack has finished. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
net.DialUDP returns a connected socket, so an ICMP port unreachable from the collector surfaces as an error on the next write. That error was propagated out of Flush and terminated the process: mode=ambient collector=127.0.0.1:6343 ... duration=infinite error: send sFlow datagram: write udp ...: connect: connection refused exit status 1 The generator therefore died ~200ms after startup if the collector was not listening yet, and any collector restart killed a running ambient profile. Treat a failed send like a lost sample: count it, drop the datagram, log the first failure and then at most one line every 10s, and log the recovery. Encoding errors stay fatal. The count is reported as send_errors in the periodic log so silent drops remain visible. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
runAmbient passed the configured tick to model.Tick while runAttack already used the measured one. time.Ticker drops ticks when the consumer falls behind, so under load ambient credited less time than actually elapsed and permanently under-generated the configured rate with no way to catch up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The max_samples_per_tick warning was logged on every affected tick, which is ten lines per second at the default tick of 100ms. Warn once and report the number of affected ticks as clipped in the periodic log line instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SampleBudget.Bytes grew without bound whenever samples could not be emitted, either because max_samples_per_tick clipped the tick or because the process was delayed. The arrears were spent later as one artificial burst. Cap the backlog at four intervals of credit, with a floor of two maximum-size frames so that a very low-rate host can still accumulate enough for a single sample. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The emission loop returned from the middle of the per-host loop once maxSamples was reached, so every host and network after that point never reached AddRate at all. Their traffic was not deferred, it was lost, and because TargetBPS is accumulated in the same loop the reported target dropped with it: the log showed target and estimated in agreement while half the configured traffic was missing. With two identical 1Gbps networks and a clip of 5 samples per tick: before: netA=100 netB=0 target=1.01Gbps (configured total 2Gbps) after: netA=52 netB=48 target=2.00Gbps Account for every host first, then emit in a separate pass that resumes from where the previous tick was cut off, so clipping is shared out across hosts and networks instead of always falling on the last ones. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
UDP error handling can hide permanent failures, while clipping distorts host and directional traffic distributions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Improves traffic generation, configuration handling, packet identity, runtime metrics, and exporter resilience.
Changes:
- Adds bounded catch-up and clipping-aware scheduling.
- Preserves explicit zero settings and stable MAC identities.
- Tracks UDP exporter failures with updated tests and documentation.
File summaries
| File | Description |
|---|---|
runner.go |
Adds measured intervals and richer metrics. |
runner_test.go |
Tests interval bounds and attack phases. |
README.md |
Documents metrics and clipping behavior. |
packet.go |
Uses resolved configuration and stable MAC roles. |
model.go |
Caps backlogs and rotates clipped emission. |
model_test.go |
Tests backlog and clipping behavior. |
exporter.go |
Tracks and tolerates UDP send failures. |
exporter_test.go |
Tests nonfatal send failures. |
config.go |
Distinguishes omitted settings from explicit zero values. |
config_test.go |
Tests zero-value and default handling. |
address.go |
Removes redundant address logic and an unused helper. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| directionIndex := (startDirection + directionOffset) % len(ambientDirections) | ||
| direction := ambientDirections[directionIndex] |
There was a problem hiding this comment.
I am able to reproduce, unclipped the same config gives 180000/20000. Same caveat as above: on main that clipped case is incoming=2000 outgoing=0, so outgoing was entirely starved. I will fold the weighted direction choice into the same follow-up as the host scheduler, since both want the quota split by credited budget.
I can open tracking issue, as this is separate feature, rather than bugfix.
Sorry for joining several quite different commits, but they are mostly small fixes and not worth separate PR.